[ISSUE #10987] Fix queryMinOffsetInAllGroup deleting consumer offsets from the live offset table by unbridled-41 · Pull Request #10991 · apache/rocketmq

unbridled-41 · 2026-08-29T17:37:24Z

Which Issue(s) This PR Fixes

Fixes [Bug] QUERY_CORRECTION_OFFSET admin query permanently deletes the filtered groups' consumer offsets #10987

Brief Description

ConsumerOffsetManager#queryMinOffsetInAllGroup(topic, filterGroups) iterated the live offsetTable.keySet() and called it.remove() on it to exclude the filter groups. Since ConcurrentHashMap.keySet() is a live view, running the read-only admin operation QUERY_CORRECTION_OFFSET (AdminBrokerProcessor#queryCorrectionOffset, exposed via DefaultMQAdminExt#queryCorrectionOffset) permanently deleted every topic@group offset entry of the filtered groups:

the in-memory offsets are gone and the next persist() makes the deletion permanent (consumerOffset.json);
with RocksDBConsumerOffsetManager, removeConsumerOffset deletes the rows from RocksDB immediately;
consumers of the filtered group then see -1 from queryOffset and re-initialize per consumeFromWhere → mass duplicate consumption or skipping to max;
topicAtGroup.split(TOPIC_GROUP_SEPARATOR)[1] also threw ArrayIndexOutOfBoundsException on a malformed key without @.

This PR makes the exclusion work on a snapshot of the key set, so the query no longer mutates offsetTable at all (and never calls removeConsumerOffset), while preserving the original filter semantics: offsets of the filter groups are excluded from the min-offset computation. The malformed-key AIOOBE is fixed by checking arrays.length == 2.

Priority

PRIORITY = 76: impact 32 (a read-only admin query permanently deletes persisted consumer offsets — in memory, consumerOffset.json, and RocksDB rows — so filtered groups re-initialize per consumeFromWhere: mass duplicate consumption or skipping to max) + scope 12 (the QUERY_CORRECTION_OFFSET admin API path of every broker) + reproducibility 18 (two deterministic regression tests, one per failure mode) + maintenance 14 (small snapshot-based fix that preserves the existing filter semantics). FIX_CONFIDENCE = 95.

How Did You Test This Change?

Two regression tests in ConsumerOffsetManagerTest (split so each failure mode is asserted independently):

testQueryMinOffsetInAllGroupDoesNotDeleteOffsets: the filtered group is excluded from the min-offset computation, its offsets remain in the table (queryOffset still returns them) after the query, and the unfiltered query still returns the cross-group minimum.
testQueryMinOffsetInAllGroupToleratesMalformedKeys: a stored key without @ no longer breaks the query.

Verified results (re-run 2026-09-05; after = branch tip 906bab3, before = base commit e348efa with the same test files):

After fix: mvn -pl broker test -Dtest=ConsumerOffsetManagerTest → 7/7 pass.
Before fix, each test fails on its own (run individually, because the project's surefire config skipAfterFailureCount=1 aborts a class run after the first error):
- testQueryMinOffsetInAllGroupDoesNotDeleteOffsets → FAILURE: Expecting actual: {"Topic@G1"={0=50L}} to contain key: "Topic@G2" — the query deleted the filtered group's entry from the live offset table.
- testQueryMinOffsetInAllGroupToleratesMalformedKeys → ERROR: java.lang.ArrayIndexOutOfBoundsException: Index 1 out of bounds for length 1.

Risk

Very low: the query now iterates a snapshot of the key set and never mutates offsetTable (hence never calls removeConsumerOffset); the filter semantics — excluding filter-group offsets from the min computation — are unchanged, and the arrays.length == 2 guard only skips keys that previously threw. No public API or persistence format change.

…ffsets from the live offset table

RockteMQ-AI

Summary

This PR fixes a critical bug where queryMinOffsetInAllGroup was mutating the live offsetTable by deleting consumer offsets for filtered groups. The original code iterated over the live keySet and called removeConsumerOffset(), which destroyed offset data. The fix works on a snapshot of keys (new HashSet<>(this.offsetTable.keySet())) and uses removeIf on the snapshot instead. A comprehensive test verifies that filtered group offsets are preserved after the query.

LGTM — excellent fix for this data corruption bug!

Automated review by github-manager-bot

…sts so offset deletion and the malformed-key AIOOBE fail independently

unbridled-41 · 2026-08-30T09:56:31Z

Test evidence (before → after)

The regression tests were verified in both directions on JDK 8 (mvn -pl broker test -Dtest=ConsumerOffsetManagerTest):

On the unfixed code (fix reverted, tests kept), the two regression tests now fail independently and each one demonstrates one defect:

Data destruction — the filtered group's offsets are gone from the live table after the query:

java.lang.AssertionError:
Expecting actual:
  {"Topic@G1"={0=50L}}
to contain key:
  "Topic@G2"
	at ...ConsumerOffsetManagerTest.testQueryMinOffsetInAllGroupDoesNotDeleteOffsets

Note the query returned while Topic@G2 (30L) was silently deleted from offsetTable; with RocksDBConsumerOffsetManager this deletion is immediately persisted to RocksDB via removeConsumerOffset, and with JSON config it is persisted by the next persist().

Malformed key handling — ArrayIndexOutOfBoundsException: Index 1 out of bounds for length 1 from topicAtGroup.split(TOPIC_GROUP_SEPARATOR)[1] on any topic@group key without @.

With this PR: Tests run: 7, Failures: 0, Errors: 0, Skipped: 0 (5 pre-existing + 2 new tests).

I split the originally single test into testQueryMinOffsetInAllGroupDoesNotDeleteOffsets and testQueryMinOffsetInAllGroupToleratesMalformedKeys (commit 906bab3) so that the offset-deletion failure is directly observable — previously the AIOOBE from the malformed key masked the deletion assertion.

Side note on CI: the workflow runs for this PR are in action_required state (first-time contributor), so they will start once a maintainer approves them. Local checkstyle baseline (style/rmq_checkstyle.xml) reports ~14.9k pre-existing violations on develop itself; the change introduces no new violation categories beyond the long-line style already used throughout these files.

RockteMQ-AI

Summary

This PR modifies 2 files (108 lines changed).

Automated scan completed. A maintainer should do a detailed review of the logic changes.

Files Changed

broker/src/main/java/org/apache/rocketmq/broker/offset/ConsumerOffsetManager.java
broker/src/test/java/org/apache/rocketmq/broker/offset/ConsumerOffsetManagerTest.java

Automated review by github-manager-bot

RockteMQ-AI

Summary

This PR fixes a critical bug where queryMinOffsetInAllGroup was mutating the live offsetTable by removing filtered groups' offsets via Iterator.remove() and removeConsumerOffset(). The fix correctly works on a HashSet snapshot and uses the filtered set as a membership guard in the subsequent iteration — no more side-effects from a read-only query.

The added tests cover both the core regression (filtered group's offsets survive the query) and the edge case of malformed keys without the @ separator.

Looks good. 👍

Automated review by github-manager-bot

codecov-commenter · 2026-09-01T07:32:38Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 48.55%. Comparing base (e348efa) to head (906bab3).

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #10991      +/-   ##
=============================================
- Coverage      48.59%   48.55%   -0.05%     
+ Complexity     13680    13669      -11     
=============================================
  Files           1381     1381              
  Lines         101475   101473       -2     
  Branches       13190    13190              
=============================================
- Hits           49313    49271      -42     
- Misses         46163    46194      +31     
- Partials        5999     6008       +9

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

RockteMQ-AI

Summary

Fixes a critical data loss bug in queryMinOffsetInAllGroup — the query operation was mutating offsetTable by removing entries during iteration, destroying consumer offset data.

Observations

✅ Works on a snapshot of keys (new HashSet<>(offsetTable.keySet())) instead of the live keySet, preventing ConcurrentModificationException and data mutation
✅ Replaces unsafe iterator.remove() with removeIf on the snapshot
✅ Removes the destructive removeConsumerOffset() call from the query path
✅ Adds proper filtering logic in the main loop to skip filtered groups
✅ Includes two comprehensive tests:
- testQueryMinOffsetInAllGroupDoesNotDeleteOffsets — verifies filtered groups' offsets are preserved
- testQueryMinOffsetInAllGroupToleratesMalformedKeys — verifies graceful handling of malformed keys

Analysis

The original code had a severe bug: a read-only query operation was destroying data by calling removeConsumerOffset() during iteration. This would cause:

Loss of consumer offset data for filtered groups
Potential ConcurrentModificationException
Incorrect min offset calculations

The fix correctly separates the filtering logic (on a snapshot) from the query logic (on the original data), ensuring the query is truly read-only.

LGTM — critical data loss bug fix.

Automated review by github-manager-bot

RockteMQ-AI

Summary

Critical data-loss bug: the read-only admin operation queryMinOffsetInAllGroup was mutating the live offsetTable by calling it.remove() on ConcurrentHashMap.keySet() (which is a live view). This silently deleted consumer offsets as a side effect of a query.

The fix correctly snapshots the key set into a HashSet before filtering, and adds a containsKey guard in the iteration loop. The removeIf with the arrays.length == 2 guard also makes the filter robust against malformed keys.

Two well-written tests: one verifies offsets survive the query, the other verifies malformed keys don't crash the operation.

LGTM.

Automated review by github-manager-bot

RockteMQ-AI

Summary

Fixes a critical data loss bug where queryMinOffsetInAllGroup() was mutating the live offsetTable through the keySet() iterator's remove(), which called removeConsumerOffset() and deleted consumer offsets as a side effect of a read-only query.

The fix:

Works on a snapshot of keys (new HashSet<>(keySet()))
Uses removeIf on the copy, not the live table
Filters via the snapshot, then iterates the live table with a containment check
Handles malformed keys (no @ separator) gracefully

Tests verify: filtered groups are excluded from min computation but their offsets are preserved; malformed keys do not break the query.

LGTM 👍

Automated review by github-manager-bot

RockteMQ-AI

Summary

Fixes a critical data loss bug where queryMinOffsetInAllGroup() was mutating the live offsetTable through the keySet() iterator's remove(), which called removeConsumerOffset() and deleted consumer offsets as a side effect of a read-only query.

The fix:

Works on a snapshot of keys (new HashSet<>(keySet()))
Uses removeIf on the copy, not the live table
Filters via the snapshot, then iterates the live table with a containment check
Handles malformed keys (no @ separator) gracefully

Tests verify: filtered groups are excluded from min computation but their offsets are preserved; malformed keys do not break the query.

LGTM 👍

Automated review by github-manager-bot

unbridled-41 · 2026-09-04T22:53:18Z

Evidence chain re-verified on 2026-09-05 (JDK 21, Maven 3.8.7), anchored to commits:

After fix — branch tip 906bab3:
mvn -pl broker test -Dtest=ConsumerOffsetManagerTest → Tests run: 7, Failures: 0, Errors: 0, Skipped: 0.
Before fix — base commit e348efa with the same regression test files (tests run individually because the project's surefire config sets skipAfterFailureCount=1, which aborts a class run after the first error):
- mvn -pl broker test -Dtest=ConsumerOffsetManagerTest#testQueryMinOffsetInAllGroupDoesNotDeleteOffsets →
  Tests run: 1, Failures: 1 — java.lang.AssertionError: Expecting actual: {"Topic@G1"={0=50L}} to contain key: "Topic@G2" (ConsumerOffsetManagerTest.java:151), i.e. the read-only query removed the filtered group's entry from the live offset table.
- mvn -pl broker test -Dtest=ConsumerOffsetManagerTest#testQueryMinOffsetInAllGroupToleratesMalformedKeys →
  Tests run: 1, Errors: 1 — java.lang.ArrayIndexOutOfBoundsException: Index 1 out of bounds for length 1 (ConsumerOffsetManagerTest.java:173).
CI: all 10 checks green on this PR (maven-compile linux/macos/windows, bazel, CodeQL, coverage, license, misspell).

[ISSUE apache#10987] Fix queryMinOffsetInAllGroup deleting consumer o…

cd89d99

…ffsets from the live offset table

RockteMQ-AI approved these changes Aug 29, 2026

View reviewed changes

[ISSUE apache#10987] Split the queryMinOffsetInAllGroup regression te…

906bab3

…sts so offset deletion and the malformed-key AIOOBE fail independently

RockteMQ-AI reviewed Aug 30, 2026

View reviewed changes

RockteMQ-AI approved these changes Aug 30, 2026

View reviewed changes

RockteMQ-AI approved these changes Sep 2, 2026

View reviewed changes

RockteMQ-AI approved these changes Sep 3, 2026

View reviewed changes

RockteMQ-AI approved these changes Sep 4, 2026

View reviewed changes

unbridled-41 marked this pull request as draft September 4, 2026 17:47

unbridled-41 marked this pull request as ready for review September 5, 2026 06:29

Navigation Menu

Uh oh!

[ISSUE #10987] Fix queryMinOffsetInAllGroup deleting consumer offsets from the live offset table - #10991

[ISSUE #10987] Fix queryMinOffsetInAllGroup deleting consumer offsets from the live offset table#10991
unbridled-41 wants to merge 2 commits into
apache:developfrom
unbridled-41:fix/query-correction-offset-destroys-offsets

unbridled-41 commented Aug 29, 2026 •

edited

Loading

Uh oh!

RockteMQ-AI left a comment

Uh oh!

unbridled-41 commented Aug 30, 2026

Uh oh!

RockteMQ-AI left a comment

Uh oh!

RockteMQ-AI left a comment

Uh oh!

codecov-commenter commented Sep 1, 2026

Uh oh!

RockteMQ-AI left a comment

Uh oh!

RockteMQ-AI left a comment

Uh oh!

RockteMQ-AI left a comment

Uh oh!

RockteMQ-AI left a comment

Uh oh!

unbridled-41 commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

unbridled-41 commented Aug 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which Issue(s) This PR Fixes

Brief Description

Priority

How Did You Test This Change?

Risk

Uh oh!

RockteMQ-AI left a comment

Choose a reason for hiding this comment

Summary

Uh oh!

unbridled-41 commented Aug 30, 2026

Test evidence (before → after)

Uh oh!

RockteMQ-AI left a comment

Choose a reason for hiding this comment

Summary

Files Changed

Uh oh!

RockteMQ-AI left a comment

Choose a reason for hiding this comment

Summary

Uh oh!

codecov-commenter commented Sep 1, 2026

Codecov Report

Uh oh!

RockteMQ-AI left a comment

Choose a reason for hiding this comment

Summary

Observations

Analysis

Uh oh!

RockteMQ-AI left a comment

Choose a reason for hiding this comment

Summary

Uh oh!

RockteMQ-AI left a comment

Choose a reason for hiding this comment

Summary

Uh oh!

RockteMQ-AI left a comment

Choose a reason for hiding this comment

Summary

Uh oh!

unbridled-41 commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

unbridled-41 commented Aug 29, 2026 •

edited

Loading